Fixes 32792: let CreateTableRequest carry certification directly - #32793
Fixes 32792: let CreateTableRequest carry certification directly#32793zak-nuccio wants to merge 6 commits into
Conversation
CreateTableRequest had no certification field, so database metadata ingestion had to create/update a table first and follow up with a separate JSON Patch to /certification once the entity existed. That patch is also invisible to sourceHash, so a source system change that is only a certification update produces the same hash as the previous run and the bulk fast-path skips it, silently dropping the update. - createTable.json: add optional certification (AssetCertification). - TableMapper: pass CreateTable.certification through to the entity. - EntityRepository.updateCertification: the bot-overwrite guard used to revert any bot PUT once a certification existed, regardless of what the request contained (a workaround for CreateTableRequest never carrying a value). Narrow it to only preserve certification when the request omits it, matching the overrideMetadata escape hatch already used for description/owners/domains on the same class. An explicit certification value from a bot is now applied instead of reverted. - generate_source_hash needs no change: it hashes the full request via model_dump(), so certification is automatically included once it is a model field. Tests: source-hash stability/change coverage in test_source_hash.py, and four EntityRepositoryCertificationTest cases exercising the guard directly (omit-preserves, explicit-applies, overrideMetadata-clears, non-bot-clears).
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
🟡 Changes recommended
The schema change is not reflected in committed UI-generated TypeScript types (e.g., generated CreateTable/CreateTableRequest models still lack certification), leaving generated artifacts out of sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR enables database ingestion to set/update table certification in the same PUT /tables request by adding certification to the CreateTable request model, ensuring certification-only changes affect sourceHash and are not skipped by the bulk fast-path.
Changes:
- Add optional
certification(AssetCertification) to theCreateTableRequestJSON schema. - Thread
certificationinto table entity creation viaTableMapper. - Refine
EntityRepository.EntityUpdater.updateCertification()to preserve existing certification only when a bot PUT omits the field (andoverrideMetadata=false), while allowing explicit bot-provided certification values. - Add Java unit tests for the updated certification guard and Python unit tests asserting
sourceHashchanges with certification payload changes.
File summaries
| File | Description |
|---|---|
| openmetadata-spec/src/main/resources/json/schema/api/data/createTable.json | Adds optional certification field to the CreateTable request schema. |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/databases/TableMapper.java | Maps request certification onto the Table entity during create/update mapping. |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java | Narrows bot PUT “preserve certification” guard to omission-only + honors overrideMetadata. |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryCertificationTest.java | Adds unit tests covering the new guard behavior (bot omit/explicit, overrideMetadata, human). |
| ingestion/tests/unit/utils/test_source_hash.py | Adds tests verifying generate_source_hash() changes/stability with certification added/changed/omitted. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "type": "string", | ||
| "minLength": 1, | ||
| "maxLength": 32 | ||
| }, | ||
| "certification": { | ||
| "description": "Certification for a table", | ||
| "$ref": "../../type/assetCertification.json" | ||
| } |
There was a problem hiding this comment.
Right, the generated TS models were stale — fixed in 56d4e5c by regenerating via json2ts-generate-all.sh (same as the TypeScript Type Generation CI workflow runs). Only createTable.ts and bulkCreateTable.ts changed, matching the schema edit.
The backend always recomputes AssetCertification.appliedDate/expiryDate server-side from AssetCertificationSettings when a certification is applied, ignoring whatever the request sent for those two fields. Only tagLabel reflects a real change. Hashing appliedDate/expiryDate would destabilize sourceHash on every ingestion run if a connector ever populates them with a run-time-relative value, defeating the bulk fast-path this PR's certification support relies on. Scoped to the certification field only (not a global key strip), since expiryDate is also a legitimate, meaningful field on regular tag metadata (TagLabelMetadata) that should stay part of the hash.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
🟡 Changes recommended
The CreateTable schema change needs the corresponding committed UI-generated TypeScript schema outputs regenerated/updated to keep spec-derived clients in sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
openmetadata-spec/src/main/resources/json/schema/api/data/createTable.json:119
- The CreateTable JSON schema now includes the new optional
certificationfield, but the committed UI-generated TypeScript types still defineCreateTablewithoutcertification. This will leave UI/client type generation out of sync with the spec; please regenerate and commit the updated TS schema outputs underopenmetadata-ui/src/main/resources/ui/src/generated/(e.g.,.../api/data/createTable.ts).
},
"certification": {
"description": "Certification for a table",
"$ref": "../../type/assetCertification.json"
}
ingestion/tests/unit/utils/test_source_hash.py:567
- This line exceeds the repo's Ruff line-length (120) and will likely fail formatting/lint checks; wrap the
AssetCertification(...)call across multiple lines.
certification=AssetCertification(tagLabel=tag_label, appliedDate=1800000000000, expiryDate=1999999999999),
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| name="test_table", | ||
| databaseSchema="service.db.schema", | ||
| columns=[Column(name="id", dataType=DataType.INT)], | ||
| certification=AssetCertification(tagLabel=tag_label, appliedDate=1700000000000, expiryDate=1731536000000), |
There was a problem hiding this comment.
Checked — both lines are 118 characters, under the 120 limit (awk '{print length}' confirms it), and ruff check/ruff format --check both pass clean on this file as committed. Not making a change here; flagging as a false positive rather than splitting these two calls across more lines.
createTable.ts and bulkCreateTable.ts were out of sync with the certification field added to createTable.json. Regenerated via json2ts-generate-all.sh, matching the TypeScript Type Generation CI workflow.
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added ingestion unit test includes lines that are likely to violate the repository’s Ruff line-length=120 constraint and can fail CI linting.
Review details
Suppressed comments (2)
ingestion/tests/unit/utils/test_source_hash.py:561
- This line is likely to exceed the repository's Ruff 120-character line-length limit, which can fail CI linting for ingestion tests. Please wrap the AssetCertification construction across multiple lines (similar to the _certification helper) to keep lines <= 120 chars.
certification=AssetCertification(tagLabel=tag_label, appliedDate=1700000000000, expiryDate=1731536000000),
ingestion/tests/unit/utils/test_source_hash.py:567
- This line is likely to exceed the repository's Ruff 120-character line-length limit, which can fail CI linting for ingestion tests. Please wrap the AssetCertification construction across multiple lines to keep lines <= 120 chars.
certification=AssetCertification(tagLabel=tag_label, appliedDate=1800000000000, expiryDate=1999999999999),
- Files reviewed: 6/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
updateCertification()'s unchanged-check compared the full AssetCertification object, including appliedDate/expiryDate. The server always recomputes those two fields when a certification is applied, so a request that legitimately can't know the server's current dates (e.g. an ingestion connector re-sending the same certification every run) would never compare equal - triggering a spurious re-apply and version bump on every non-bulk PUT even when the certification itself hadn't changed. Compare by tagLabel.tagFQN instead, matching the identity check applyCertification() already uses for its own idempotency.
|
Re: the version-churn edge case in Gitar's latest review — good catch, fixed in 1fd6ca5. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
🟡 Changes recommended
updateCertification() returns early when the certification tag is unchanged without restoring the stored certification, allowing request-supplied date fields to be persisted without a version bump/audit record.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/8 changed files
- Comments generated: 2
- Review effort level: Lite
| if (certificationTagUnchanged) { | ||
| LOG.debug("Certification unchanged"); | ||
| return; | ||
| } |
| invokeUpdateCertification(updater); | ||
|
|
||
| assertEquals(4000000000000L, updated.getCertification().getAppliedDate()); | ||
| verify(tagUsageDAO, never()) |
The tagLabel-only unchanged check skipped re-applying the certification but left the request's arbitrary appliedDate/expiryDate on the updated entity, which the caller then persists regardless of whether this method recorded a change. Restore the stored (server-authoritative) certification, including its real dates, before returning, so the client's date fields can never silently overwrite storage without a version bump or audit record.
|
Good catch, fixed in 03c28a2. The tag-unchanged early return skipped re-applying the certification but left the request's own appliedDate/expiryDate on |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are cohesive, well-tested across backend and ingestion hashing behavior, and the remaining feedback is a minor comment-clarity nit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java:10057
- The comment says we compare "by tagLabel only", but the actual comparison is only on
tagLabel.tagFQN(not the full TagLabel). Updating the comment will prevent confusion about what changes are considered "unchanged" (e.g., labelType/state differences are ignored).
- Files reviewed: 6/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Fair, fixed in 499283a — comment now says tagLabel.tagFQN explicitly and notes that other TagLabel fields (labelType, state, etc.) are intentionally ignored for this comparison. |
|
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Code Review ✅ Approved 2 resolved / 2 findingsAdds optional ✅ 2 resolved✅ Edge Case: Certification appliedDate/expiryDate can destabilize sourceHash
✅ Edge Case: Bot-supplied certification re-applies and bumps version each run
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Describe your changes:
Fixes #32792
CreateTableRequesthad nocertificationfield, so database metadata ingestion had to create/update a table first and follow up with a separate JSON Patch to/certification. That patch is invisible tosourceHash, so a source-system change that is only a certification update produces the same hash as the previous run and the bulk fast-path silently skips it.This adds an optional
certification(AssetCertification) field toCreateTableRequest, threads it throughTableMapper, and narrows the bot-overwrite guard inEntityRepository.updateCertificationso a bot's explicit certification value is applied instead of always being reverted.Type of change:
High-level design:
EntityRepository.updateCertification()had a guard that reverted any bot PUT once a certification existed, regardless of what the request contained — a workaround forCreateTableRequestnever carrying a certification value, so every bot PUT implicitly meant "no certification" and would otherwise have wiped it on every re-sync.Now that a bot can supply an explicit value, the guard only needs to protect against omission:
This mirrors the
overrideMetadataescape hatch already used fordescription/owners/domainson the same class, rather than introducing a new mechanism:overrideMetadata=true-> bypasses the guard entirely, consistent with the other protected fields.No change was needed to
generate_source_hash(): it hashes the full request viamodel_dump(), so certification is automatically included in the hash once it's a model field.Alternatives considered:
certificationfromsourceHashas a special case — rejected, since a certification-only source change would still look "unchanged" to the bulk fast path.sourceHashblind spot.Tests:
Use cases covered
CreateTableRequest.certificationon table create -> certification is applied.certificationon a later run -> existing certification (e.g. set through the UI) is preserved, not wiped.certificationvalue on a later run -> the new value is applied andsourceHashdiffers, so the bulk fast-path doesn't skip it.overrideMetadata=truestill clears certification when the request omits it, matching existing behaviour for other protected fields.Unit tests
openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/EntityRepositoryCertificationTest.java— 4 new tests exercisingupdateCertification()'s guard directly (bot-omit-preserves, bot-explicit-applies, bot-omit-with-overrideMetadata-clears, human-omit-clears).ingestion/tests/unit/utils/test_source_hash.py— certification-added/changed/omitted/equivalent-payload hash stability tests forgenerate_source_hash.Backend integration tests
PUT /tablesalready acceptsCreateTableRequest).Ingestion integration tests
Manual testing performed
EntityRepositoryCertificationTestsuite (32/32 passing) and the fullorg.openmetadata.service.jdbi3.**package (616/616 passing).ingestion/tests/unit/utils/test_source_hash.pyandingestion/tests/unit/topology/test_runner.py(all passing) after regenerating Python models from the updated schema.mvn spotless:applyandmvn spotless:checkclean onopenmetadata-service.UI screen recording / screenshots:
Not applicable.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.certificationis a new optional field onCreateTableRequest, so no migration is needed (existing stored data is unaffected).